fix problem that NewReplicaSet shows <none> when describing deployments #97752

borgerli · 2021-01-06T03:45:26Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #97750

Special notes for your reviewer:
The cause of issue #97750 is that the sort.Sort calls in describeDeployment->DescribePodTemplate->describeContainers->describeContainerVolumes could change the order of the items in VolumeMounts array of containers, which will fail the equalIgnoreHash comparison in findNewReplicaSet. As a result, no new replicaset could be found, and the NewReplicaSet will show as .

So the fix is just pass the deep copy of the deployment object when calling describeDeployment

k8s-ci-robot · 2021-01-06T03:45:35Z

Hi @borgerli. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rikatz · 2021-01-06T20:39:02Z

/cc
/ok-to-test

borgerli · 2021-01-07T02:21:56Z

/retest

borgerli · 2021-01-07T07:59:19Z

/assign @eddiezane

rikatz · 2021-01-07T19:29:12Z

Please add the Release Note block
As this generated the issue NewReplicaSet shows <none> when describing deployment #97750 can you please improve the unit test for TestDescribeDeployment? My suggestion is to follow the TestDescribeIngressNetworkPolicies func, maybe having some improvements in the tests with the expected output other then only if the command runs or not would be really better

@eddiezane Other than the above, lgtm. Will wait your opinion about improving the unit tests so future bugs can be catched earlier :)

borgerli · 2021-01-08T02:57:30Z

Please add the Release Note block

As this generated the issue NewReplicaSet shows <none> when describing deployment #97750 can you please improve the unit test for TestDescribeDeployment? My suggestion is to follow the TestDescribeIngressNetworkPolicies func, maybe having some improvements in the tests with the expected output other then only if the command runs or not would be really better

@eddiezane Other than the above, lgtm. Will wait your opinion about improving the unit tests so future bugs can be catched earlier :)

Thanks for the comment. I'll add the unit test.

borgerli · 2021-01-08T11:55:18Z

@rikatz I just modified the test case of TestDescribeDeployment by covering the NewReplicaSet info verification. Please review. Thanks.

rikatz · 2021-01-08T17:42:57Z

@borgerli Thanks, will review.

Can you please edit the RELEASE NOTE block on the description of the PR and add a "NONE" into that?

Thanks

rikatz

@borgerli thank you again.

I've left one review about the test. To be honest, I would expect that we test the whole output instead of only one field, so we can detect other deviations / bugs caused by future changes, there's some example on the test I've mentioned before.

If you need some help with that please let me know.

Thank you!

rikatz · 2021-01-08T18:06:55Z

staging/src/k8s.io/kubectl/pkg/describe/describe_test.go

@@ -1719,6 +1806,10 @@ func TestDescribeDeployment(t *testing.T) {
 	if !strings.Contains(out, "bar") || !strings.Contains(out, "foo") || !strings.Contains(out, "Containers:") || !strings.Contains(out, "mytest-image:latest") {
 		t.Errorf("unexpected out: %s", out)
 	}
+	expectedNewReplicaSet := "NewReplicaSet:   bar-001 (1/1 replicas created)"
+	if !strings.Contains(out, expectedNewReplicaSet) {


OK, this seems a good approach. I'm mostly biased to have a comparison between the full output instead of only the NewReplicaSet string, so we can validate the behavior of the whole describe command instead of only one line (we want to get all divergences instead of only this line, right?)

Thank you!

Yes, agreed. I'll improve the test case to validate all computed fields. Thank you.

borgerli · 2021-01-09T14:00:42Z

/release-note-none

rikatz · 2021-01-13T12:41:19Z

@borgerli let me know when the tests are ready :)

borgerli · 2021-01-14T01:57:56Z

@rikatz Sorry that I'm busy with work this week. I'll try to get tests ready by this Sunday.

borgerli · 2021-01-16T09:33:16Z

/retest

borgerli · 2021-01-16T10:24:22Z

@rikatz I improved the test case. Please help review. Thanks.

rikatz · 2021-01-19T15:31:40Z

Will review today

rikatz · 2021-01-19T16:46:59Z

/triage accepted
/priority important-soon

rikatz · 2021-01-19T16:59:02Z

/lgtm

Thanks!

rikatz · 2021-01-19T17:11:30Z

/cc

pwittrock · 2021-01-20T15:57:53Z

/approve

k8s-ci-robot · 2021-01-20T15:58:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: borgerli, pwittrock

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/kubectl/OWNERS~~ [pwittrock]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rikatz · 2021-01-20T16:21:25Z

/retest Em qua., 20 de jan. de 2021 às 13:09, Kubernetes Prow Robot < notifications@github.com> escreveu:

…

@borgerli <https://github.com/borgerli>: The following test *failed*, say /retest to rerun all failed tests: Test name Commit Details Rerun command pull-kubernetes-bazel-build a286455 <a286455> link <https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/97752/pull-kubernetes-bazel-build/1351922938080661504> /test pull-kubernetes-bazel-build Full PR test history <https://prow.k8s.io/pr-history?org=kubernetes&repo=kubernetes&pr=97752>. Your PR dashboard <https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Aborgerli>. Please help us cut down on flakes by linking to <https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#97752 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABWZQBIVUDDGTKJKXNMFCQTS2353JANCNFSM4VW3PITA> .

rikatz · 2021-01-20T17:02:03Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

k8s-ci-robot requested review from deads2k and zhouya0 January 6, 2021 03:46

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 6, 2021

k8s-ci-robot requested a review from rikatz January 6, 2021 20:39

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 6, 2021

k8s-ci-robot assigned eddiezane Jan 7, 2021

borgerli force-pushed the master branch from 178a032 to 157f0b7 Compare January 8, 2021 11:51

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 8, 2021

rikatz suggested changes Jan 8, 2021

View reviewed changes

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 9, 2021

borgerli force-pushed the master branch from 157f0b7 to f0c75dd Compare January 16, 2021 05:14

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 16, 2021

fix problem that NewReplicaSet shows <none> when describing deployments

a286455

borgerli force-pushed the master branch from f0c75dd to a286455 Compare January 16, 2021 06:20

k8s-ci-robot assigned rikatz Jan 19, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2021

k8s-ci-robot requested a review from rikatz January 19, 2021 17:11

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2021

k8s-ci-robot merged commit 0d5a368 into kubernetes:master Jan 20, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix problem that NewReplicaSet shows <none> when describing deployments #97752

fix problem that NewReplicaSet shows <none> when describing deployments #97752

borgerli commented Jan 6, 2021 •

edited

k8s-ci-robot commented Jan 6, 2021

rikatz commented Jan 6, 2021

borgerli commented Jan 7, 2021

borgerli commented Jan 7, 2021

rikatz commented Jan 7, 2021

borgerli commented Jan 8, 2021

borgerli commented Jan 8, 2021

rikatz commented Jan 8, 2021

rikatz left a comment

rikatz Jan 8, 2021

borgerli Jan 9, 2021 •

edited

borgerli commented Jan 9, 2021

rikatz commented Jan 13, 2021

borgerli commented Jan 14, 2021

borgerli commented Jan 16, 2021

borgerli commented Jan 16, 2021

rikatz commented Jan 19, 2021

rikatz commented Jan 19, 2021

rikatz commented Jan 19, 2021

rikatz commented Jan 19, 2021

pwittrock commented Jan 20, 2021

k8s-ci-robot commented Jan 20, 2021

rikatz commented Jan 20, 2021 via email

rikatz commented Jan 20, 2021

fix problem that NewReplicaSet shows <none> when describing deployments #97752

fix problem that NewReplicaSet shows <none> when describing deployments #97752

Conversation

borgerli commented Jan 6, 2021 • edited

k8s-ci-robot commented Jan 6, 2021

rikatz commented Jan 6, 2021

borgerli commented Jan 7, 2021

borgerli commented Jan 7, 2021

rikatz commented Jan 7, 2021

borgerli commented Jan 8, 2021

borgerli commented Jan 8, 2021

rikatz commented Jan 8, 2021

rikatz left a comment

Choose a reason for hiding this comment

rikatz Jan 8, 2021

Choose a reason for hiding this comment

borgerli Jan 9, 2021 • edited

Choose a reason for hiding this comment

borgerli commented Jan 9, 2021

rikatz commented Jan 13, 2021

borgerli commented Jan 14, 2021

borgerli commented Jan 16, 2021

borgerli commented Jan 16, 2021

rikatz commented Jan 19, 2021

rikatz commented Jan 19, 2021

rikatz commented Jan 19, 2021

rikatz commented Jan 19, 2021

pwittrock commented Jan 20, 2021

k8s-ci-robot commented Jan 20, 2021

rikatz commented Jan 20, 2021 via email

rikatz commented Jan 20, 2021

borgerli commented Jan 6, 2021 •

edited

borgerli Jan 9, 2021 •

edited